Validation for CRD custom resources: feature gate promotion alpha->beta #54647

colemickens · 2017-10-26T19:31:45Z

What this PR does / why we need it: This promotes CRD Validation from alpha to beta.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #53829

Special notes for your reviewer: Issue #53829 discusses potential blockers to promoting CRD Validation to beta. None of the potential blockers are actual blockers, as they can all be accomplished without backward incompatible changes.

Release note:

Promote validation for custom resources defined through CRD to beta

cc: @sttts @nikhita @mbohlool

colemickens · 2017-10-26T19:32:06Z

staging/src/k8s.io/apiextensions-apiserver/pkg/features/kube_features.go

@@ -29,6 +29,7 @@ const (

 	// owner: @sttts, @nikhita
 	// alpha: v1.8
+	// beta: v1.9


not sure of the expected/desired syntax here; let me know what's appropriate.

seems right as per:

kubernetes/staging/src/k8s.io/apiserver/pkg/features/kube_features.go

Lines 59 to 65 in 7914aed

// owner: @smarterclayton

// alpha: v1.8

// beta: v1.9

//

// Allow API clients to retrieve resource lists in chunks rather than

// all at once.

APIListChunking utilfeature.Feature = "APIListChunking"

nikhita · 2017-10-26T19:37:02Z

~~This also needs update in the feature gate in the strategy and validation.~~ We enable the feature gate by default so it doesn't really affect this.

Also, in comments in types.go. Let me make a list of places it needs update. 👍

nikhita · 2017-10-26T23:00:21Z

This just needs update in a couple of places:

The removal of the line // This field is alpha-level... in:

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/types.go

Lines 31 to 34 in 7914aed

    
           // Validation describes the validation methods for CustomResources 
        
           // This field is alpha-level and should only be sent to servers that enable the CustomResourceValidation feature. 
        
           // +optional 
        
           Validation *CustomResourceValidation `json:"validation,omitempty" protobuf:"bytes,5,opt,name=validation"`

After that, you'll need to run ./hack/update-generated-protobuf.sh too.

nit: can you remove this in the tests (since the feature gate is enabled by default)? or at least the word "alpha".

kubernetes/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go

Lines 206 to 210 in 7914aed

    
           // enable alpha feature CustomResourceValidation 
        
           err = utilfeature.DefaultFeatureGate.Set("CustomResourceValidation=true") 
        
           if err != nil { 
        
           	t.Errorf("failed to enable feature gate for CustomResourceValidation: %v", err) 
        
           }

nikhita · 2017-10-26T23:05:23Z

IMO this should be called "CustomResource validation" instead of "CRD Validation" (especially in the release note). We define the schema in the CRD but we are validating the custom resources, not the CRD itself.

enisoc · 2017-10-27T17:03:30Z

@nikhita wrote:

IMO this should be called "CustomResource validation" instead of "CRD Validation" (especially in the release note). We define the schema in the CRD but we are validating the custom resources, not the CRD itself.

Technically, I think it should be "Validation for custom resources defined through CRD". The term custom resource on its own refers to the general concept of non-native, dynamically-installed resources. CRD is only one way to create custom resources; for example, this doesn't apply to resources added through API server aggregation.

nikhita · 2017-10-27T17:07:28Z

Technically, I think it should be "Validation for custom resources defined through CRD".

👍

colemickens · 2017-10-27T17:37:50Z

Thanks for the feedback! Updated the release note, PR title accordingly. I've also updated the few "alpha" references in comments. PTAL

nikhita · 2017-10-27T17:46:58Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/types.go

@@ -29,7 +29,6 @@ type CustomResourceDefinitionSpec struct {
 	// Scope indicates whether this resource is cluster or namespace scoped.  Default is namespaced
 	Scope ResourceScope `json:"scope" protobuf:"bytes,4,opt,name=scope,casttype=ResourceScope"`
 	// Validation describes the validation methods for CustomResources
-	// This field is alpha-level and should only be sent to servers that enable the CustomResourceValidation feature.


This needs update in the generated proto file too:

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/generated.proto

Lines 107 to 111 in 4eadfbb

// Validation describes the validation methods for CustomResources

// This field is alpha-level and should only be sent to servers that enable the CustomResourceValidation feature.

// +optional

optional CustomResourceValidation validation = 5;

}

nikhita · 2017-10-27T17:47:29Z

staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go

@@ -203,7 +203,7 @@ func TestCustomResourceUpdateValidation(t *testing.T) {
 	}
 	defer close(stopCh)

-	// enable alpha feature CustomResourceValidation
+	// enable feature CustomResourceValidation


There are a few other places in this file with this line. Can you update them too? :)

nikhita · 2017-10-27T19:21:35Z

The release note needs update. Maybe Promote validation for custom resources defined through CRD to beta.

Otherwise lgtm. @sttts?

sttts · 2017-10-27T19:47:22Z

staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go

@@ -176,7 +176,7 @@ func TestCustomResourceValidation(t *testing.T) {
 	}
 	defer close(stopCh)

-	// enable alpha feature CustomResourceValidation
+	// enable feature CustomResourceValidation
 	err = utilfeature.DefaultFeatureGate.Set("CustomResourceValidation=true")


can set block be dropped because it is enabled by default?

I dropped the various feature gate set blocks and amended the commit. Thanks.

sttts · 2017-10-27T19:48:10Z

Lgtm. Only the question about setting the feature gate to true in integration tests.

sttts · 2017-10-27T19:48:55Z

/approve

nikhita · 2017-10-27T20:06:36Z

/lgtm

nikhita · 2017-10-27T20:58:42Z

utilfeature does not need to be imported anymore: https://github.com/colemickens/kubernetes/blob/6e157d69ba36563b18fc06713190ee10d299c3db/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go#L28.

Funny why the CI didn't complain.

colemickens · 2017-10-28T02:16:28Z

Argh, looks like the /lgtm got canceled, if you can reapply @nikhita?

sttts · 2017-10-28T06:50:10Z

Funny why the CI didn't complain.

There is a godep issue I believe. @caesarxuchao also saw something yesterday.

sttts · 2017-10-28T06:50:19Z

/lgtm

sttts · 2017-11-01T10:48:45Z

/retest

sttts · 2017-11-01T10:48:56Z

@smarterclayton is this approved?

colemickens · 2017-11-10T22:31:24Z

/test pull-kubernetes-unit

colemickens · 2017-11-10T22:44:55Z

(no material changes yet, just a rebase while trying to get the unit tests to pass in CI)

nikhita · 2017-11-10T23:08:50Z

/lgtm

colemickens · 2017-11-10T23:28:39Z

Alright, must have just been a weird test flake (very coincidentally in the TestCRD I guess...).

Things look green, just waiting on approval. cc: @smarterclayton @sttts.

sttts · 2017-11-13T09:33:34Z

@lavalamp can you approve?

colemickens · 2017-11-14T22:09:05Z

ping @lavalamp

lavalamp · 2017-11-15T23:03:18Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/types.go

@@ -29,7 +29,6 @@ type CustomResourceDefinitionSpec struct {
 	// Scope indicates whether this resource is cluster or namespace scoped.  Default is namespaced
 	Scope ResourceScope `json:"scope" protobuf:"bytes,4,opt,name=scope,casttype=ResourceScope"`
 	// Validation describes the validation methods for CustomResources
-	// This field is alpha-level and should only be sent to servers that enable the CustomResourceValidation feature.
 	// +optional
 	Validation *CustomResourceValidation `json:"validation,omitempty" protobuf:"bytes,5,opt,name=validation"`


Was this cleared before storage if the flag was off?

If not, you have a bunch of clusters out there with time-bombs.

To check my understanding, this is a time-bomb if someone has submitted resources with validation enabled, but without having the feature actually enabled? Hence if we activate it by default in 1.8, they could have custom resources in the API that do not actually validate?

If that understanding is not correct then I'm not sure I'm groking the problem.

I can investigate whether or not the field was being cleared. If it weren't being cleared, what are our options for moving forward and/or minimizing breakage short of changing the field name?

Was this cleared before storage if the flag was off?

Yes, it was cleared if the feature was disabled. https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/strategy.go#L57

lavalamp · 2017-11-16T08:00:26Z

/approve

k8s-github-robot · 2017-11-16T08:36:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: colemickens, lavalamp, nikhita, sttts

Associated issue: 53829

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~api/OWNERS~~ [lavalamp]
~~pkg/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [lavalamp,sttts]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

nikhita · 2017-11-16T14:18:40Z

pinging to wake up the bot (it says PR does not have approved label even though it exists).

k8s-github-robot · 2017-11-16T14:32:31Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-11-16T15:19:01Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 26, 2017

colemickens commented Oct 26, 2017

View reviewed changes

k8s-github-robot assigned smarterclayton and enisoc Oct 26, 2017

colemickens mentioned this pull request Oct 26, 2017

Getting CRD Validation to Beta #53829

Closed

nikhita mentioned this pull request Oct 26, 2017

Custom Resource Validation: remove alpha warning, change enable instructions to disable kubernetes/website#6066

Merged

colemickens changed the title ~~CRD validation: feature gate promotion alpha->beta~~ CustomResource Validation: feature gate promotion alpha->beta Oct 27, 2017

colemickens force-pushed the crd-validation-beta branch from d264e21 to 93c9889 Compare October 27, 2017 17:36

colemickens changed the title ~~CustomResource Validation: feature gate promotion alpha->beta~~ Validation for CRD custom resources: feature gate promotion alpha->beta Oct 27, 2017

nikhita reviewed Oct 27, 2017

View reviewed changes

colemickens force-pushed the crd-validation-beta branch from 93c9889 to 72fe89f Compare October 27, 2017 18:22

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 27, 2017

sttts reviewed Oct 27, 2017

View reviewed changes

colemickens force-pushed the crd-validation-beta branch from 72fe89f to 6e157d6 Compare October 27, 2017 19:53

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 27, 2017

k8s-ci-robot assigned nikhita Oct 27, 2017

colemickens force-pushed the crd-validation-beta branch from 6e157d6 to 9a57753 Compare October 27, 2017 21:05

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 27, 2017

colemickens force-pushed the crd-validation-beta branch from 9a57753 to eac7004 Compare October 27, 2017 22:34

k8s-ci-robot assigned sttts Oct 28, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 28, 2017

validation of CRD custom resources: alpha->beta

ae88efb

colemickens force-pushed the crd-validation-beta branch from eac7004 to ae88efb Compare November 10, 2017 22:32

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2017

lavalamp reviewed Nov 15, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 16, 2017

k8s-github-robot merged commit 321b36c into kubernetes:master Nov 16, 2017

markmandel mentioned this pull request Dec 7, 2017

GameServer definition validation googleforgames/agones#10

Closed

	// owner: @smarterclayton
	// alpha: v1.8
	// beta: v1.9
	//
	// Allow API clients to retrieve resource lists in chunks rather than
	// all at once.
	APIListChunking utilfeature.Feature = "APIListChunking"

	// Validation describes the validation methods for CustomResources
	// This field is alpha-level and should only be sent to servers that enable the CustomResourceValidation feature.
	// +optional
	optional CustomResourceValidation validation = 5;
	}

Validation for CRD custom resources: feature gate promotion alpha->beta #54647

Validation for CRD custom resources: feature gate promotion alpha->beta #54647

Conversation

colemickens commented Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhita commented Oct 26, 2017 • edited Loading

nikhita commented Oct 26, 2017

nikhita commented Oct 26, 2017 • edited Loading

enisoc commented Oct 27, 2017

nikhita commented Oct 27, 2017

colemickens commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhita commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Oct 27, 2017

sttts commented Oct 27, 2017

nikhita commented Oct 27, 2017

nikhita commented Oct 27, 2017

colemickens commented Oct 28, 2017

sttts commented Oct 28, 2017

sttts commented Oct 28, 2017

sttts commented Nov 1, 2017

sttts commented Nov 1, 2017

colemickens commented Nov 10, 2017

colemickens commented Nov 10, 2017

nikhita commented Nov 10, 2017

colemickens commented Nov 10, 2017

sttts commented Nov 13, 2017

colemickens commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Nov 16, 2017

k8s-github-robot commented Nov 16, 2017

nikhita commented Nov 16, 2017

k8s-github-robot commented Nov 16, 2017

k8s-github-robot commented Nov 16, 2017

colemickens commented Oct 26, 2017 •

edited

Loading

nikhita commented Oct 26, 2017 •

edited

Loading

nikhita commented Oct 26, 2017 •

edited

Loading